-
Notifications
You must be signed in to change notification settings - Fork 267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSV bills import (cospend compatible) #951
CSV bills import (cospend compatible) #951
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am glad to see more Cospend compat code 👍
However, I have mixed feelings about the new __init__
for Bill
. In some case, it is helpful, but I feel that it is sometimes less readable than previous code with explicit affectations.
Great move though for replacing fake_form
by an explicit export
.
As a last comment, you can remove all translations from this, Weblate will handle this for us.
ihatemoney/models.py
Outdated
Bill( | ||
amount, | ||
None, | ||
None, | ||
"XXX", | ||
[members[name] for name in owers], | ||
members[payer].id, | ||
project.default_currency, | ||
what, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the __init__
is somewhat confusing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified init with named parameters, if it's still a problem I can revert it
That's cool! Thanks for taking the time to implement this :-) I wonder if this will break importing previously exported project data? |
As far as I tested nope, same behavior as for json : only add missing transactions and persons |
Hey, let us know if you need some help on this, we might be able to find some time to take over if you want! |
Hi, I'm planning to update this in the following days when I get some time, but for this PR or any future one feel completely free to update it ! |
00ac1b9
to
4be27b1
Compare
4be27b1
to
7d3d669
Compare
Now with CSV tests I believe this PR is ready for review :) I'm not really satisfied with the CSV/JSON tests implementation but I could not find a better way without converting to pytest, btw I'm wondering why this project uses unittest instead ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this and tidying up the tests during the process :-) I believe we're not using py.test because we are not familiar with it, but if you want and know how to improve these tests with py.test it will be useful and appreciated, don't hesitate to open a PR (or an issue) about it.
I've not reviewed all the part with the tests due to lack of time right now, but I've left a few comments here and there.
ihatemoney/models.py
Outdated
db.session.commit() | ||
|
||
# Import bills not already in the project | ||
bills_project = self.get_pretty_bills() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow here: this is named bills_project
but it seems to contain bills instead, right? If that's the case, we should probably rename the variable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand. It seems to be the bills of the existing project. If this is the case, we could name it project_bills
:-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok !
# Create bills | ||
db.session.add( | ||
Bill( | ||
amount=b["amount"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably should check that the data contains this key before using it, otherwise it may fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we might want to try/catch this entire block and display an error message with the format of the required data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import_bills is already called from a try/catch block in web.import_project
but I agree we should add another one in import_bills as a safeguard in case we use the method elsewhere
done !
ihatemoney/utils.py
Outdated
reader = csv.DictReader(csv_file) | ||
result = [] | ||
for r in reader: | ||
# cospend filtering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please expand here on why this is required and what's it's doing? Thanks :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup
by the way I thought about also exporting data in cospend format (with lines for users and a table for categories)
it would simplify our csv import code AND render IHM exports compatible with cospend
pytest would indeed simplify code and avoid some ugly duplication, but I honestly don't have the motivation to refactor all tests. I might write new ones with it though if there is no reason not to do so Thanks for your feedbacks I'll check these later this week |
ihatemoney/forms.py
Outdated
return Bill( | ||
float(self.amount.data), | ||
self.date.data, | ||
self.external_link.data, | ||
str(self.original_currency.data), | ||
Person.query.get_by_ids(self.payed_for.data, project), | ||
self.payer.data, | ||
project.default_currency, | ||
self.what.data, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be good to also use named parameters here for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, forgot to include them here, fixed !
@Youe Graillot commented on 13 déc. 2021, 04:34 UTC+1:
But… this project uses pytest: https://github.com/spiral-project/ihatemoney/blob/master/setup.cfg#L57 And I'm running all tests locally with |
Running tests with pytest does not mean they are written using pytest, which can run unittest but with caveats. |
Is this ready for review? |
I believe it's alright from my point of view. As soon as we get the green light from @youegraillot, I believe we should merge :-) |
Oups, I didn't notified you last week I think I have answered all comments, light is green ! |
Thanks a bunch! |
My pleasure, I'll move on to #152 then |
* proper import form (fix messy errors) * csv compatible import * cospend compatible import * localization (best effort) * refactoring * revert localization (best effort) * import return 400 on error * fix Person.query.get_by_ids calls * Bill explicit init parameters * fix tests * refacto tests with self.get_project * separate import tests * fix tests * csv import test case * fix import csv parsing * revert DestructiveActionProjectForm renaming * fix csv import test * fix error redirection on import * fix lint * import file input type hint * various fixes from review Co-authored-by: Youe Graillot <youe.graillot@gmail.com>
Hi, bigger pr this time to implement CSV project import
This is a draft for now, I'm planning to update and add some more tests for this feature